[Fix] Apply preset setpoints when changing active preset#43587
Open
lboue wants to merge 9 commits intoproject-chip:masterfrom
Open
[Fix] Apply preset setpoints when changing active preset#43587lboue wants to merge 9 commits intoproject-chip:masterfrom
lboue wants to merge 9 commits intoproject-chip:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request correctly implements the application of setpoints when a thermostat preset is activated. The changes in thermostat-server-presets.cpp ensure that when SetActivePreset is called with a valid preset handle, the corresponding heating and cooling setpoints are retrieved and applied to the occupied setpoint attributes. The addition of unit tests in TestThermostatServerPresets.cpp validates this new behavior effectively. I've made one suggestion to improve code efficiency by removing a redundant check and to adhere to repository guidelines for ignoring return values. Overall, this is a good fix.
src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Outdated
Show resolved
Hide resolved
lboue
force-pushed
the
fix/thermostat-apply-setpoints-on-preset-change
branch
from
52c3aca to
ff9378b
Compare
lboue
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Thermostat SetActivePreset behavior so changing the active preset also applies the preset’s occupied heating/cooling setpoints (with limit enforcement) before updating ActivePresetHandle, and improves error/status handling for invalid handles and failed setpoint writes.
Changes:
- Lookup the requested preset once (validation + data retrieval) and apply its occupied setpoints before updating
ActivePresetHandle. - Return
Status::InvalidCommandwhen the requested preset handle is not present. - Propagate (and log) the underlying status if writing
OccupiedCoolingSetpoint/OccupiedHeatingSetpointfails.
src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Outdated
Show resolved
Hide resolved
…g for cooling and heating setpoints
…ove error handling
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Thermostat preset activation behavior so that switching the active preset also applies any preset cooling/heating setpoints before updating ActivePresetHandle, and improves error handling for invalid preset handles and attribute write failures.
Changes:
- Refactors preset lookup into a single pass (
GetMatchingPresetInPresets) and uses it for both validation and setpoint retrieval. - Updates
SetActivePresetto applyOccupiedCoolingSetpoint/OccupiedHeatingSetpoint(with enforced limits) before settingActivePresetHandle, returning/logging the underlying write status on failure. - Returns
Status::InvalidCommandwhen the requested preset handle is not found.
… and AppendPendingPreset methods
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Thermostat presets server logic so switching the active preset also applies any preset-provided occupied heating/cooling setpoints before updating the ActivePresetHandle, improving behavioral consistency and reducing redundant preset list iteration.
Changes:
- Refactors preset lookup into a single
GetMatchingPresetInPresets(...)that returnsCHIP_ERRORplus afoundflag. - Updates
ThermostatAttrAccess::SetActivePresetto validate the handle via lookup, apply optional preset setpoints, and returnInvalidCommandwhen the handle is not found. - Updates
AppendPendingPresetto use the new lookup API and to distinguish lookup errors from “not found”.
|
PR #43587: Size comparison from 2403f0a to 16082d7 Full report (6 builds for cc32xx, nrfconnect, realtek, stm32)
|
…or setpoints Replace emAfWriteAttributeExternal with ZCL_INT16S_ATTRIBUTE_TYPE by the generated OccupiedCoolingSetpoint::Set / OccupiedHeatingSetpoint::Set accessors, which use the correct attribute type (temperature) registered in the metadata, fixing the INVALID_DATA_TYPE error on SetActivePreset. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Thermostat Presets server logic so that switching ActivePresetHandle also applies the preset’s occupied heating/cooling setpoints, and improves error handling when the requested preset handle is invalid or when writes fail.
Changes:
- Refactors preset lookup to a single
GetMatchingPresetInPresetspass that returnsCHIP_ERROR+found. - In
ThermostatAttrAccess::SetActivePreset, applies preset occupied setpoints (withEnforce*SetpointLimits) before updatingActivePresetHandle, returning the underlying write status on failure. - Returns
Status::InvalidCommandwhen the requested preset handle is not present.
Comments suppressed due to low confidence (1)
src/app/clusters/thermostat-server/thermostat-server-presets.cpp:357
SetActivePresetapplies occupied setpoints before callingdelegate->SetActivePresetHandle. IfSetActivePresetHandlefails, the command returns an error but the occupied setpoints may already have been updated, leaving the thermostat in a partially-applied state (and possibly with ActivePresetHandle cleared to null by the attribute-changed callback). Consider either (a) snapshotting the previous setpoints and rolling them back on failure, or (b) performing the ActivePresetHandle update in a way that can’t fail after setpoints are applied (e.g., ensure persistence before writes, or apply changes atomically in the delegate).
CHIP_ERROR err = delegate->SetActivePresetHandle(presetHandle);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "Failed to set ActivePresetHandle with error %" CHIP_ERROR_FORMAT, err.Format());
return StatusIB(err).mStatus;
}
src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
+329
to
+335
| int16_t constrainedCoolingSetpoint = EnforceCoolingSetpointLimits(coolingSetpointValue.Value(), endpoint); | ||
| Status status = OccupiedCoolingSetpoint::Set(endpoint, constrainedCoolingSetpoint); | ||
| if (status != Status::Success) | ||
| { | ||
| ChipLogError(Zcl, "Failed to set OccupiedCoolingSetpoint with status %u", to_underlying(status)); | ||
| return status; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I addressed the re-entrancy concern directly in SetActivePreset by adding an internal guard for preset-application scope.
When setpoint writes triggered during preset application cause a callback-driven SetActivePreset(nullptr), that nested clear is now ignored, so we avoid transient clears/extra writes while preserving normal explicit clear behavior outside this flow.
I also kept the existing error propagation behavior (StatusIB(...)) for lookup/write failures.
src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43587 +/- ##
==========================================
+ Coverage 54.07% 54.11% +0.03%
==========================================
Files 1548 1550 +2
Lines 106709 106975 +266
Branches 13308 13322 +14
==========================================
+ Hits 57704 57888 +184
- Misses 49005 49087 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #43587: Size comparison from 2403f0a to 1d2a162 Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Sum1cares
approved these changes
Mar 14, 2026
Sum1cares
approved these changes
Mar 14, 2026
Sum1cares
approved these changes
Mar 14, 2026
Sum1cares
approved these changes
Mar 14, 2026
Sum1cares
approved these changes
Mar 14, 2026
Sum1cares
approved these changes
Mar 14, 2026
Contributor
|
@lboue could you update the summary on why adding an automated test for this is not possible? Are we unable to add a test even via python automation tests? We are trying hard to encourage automated testing |
Contributor
Author
I'd really like to add a test but I am not a member of the CSA, so I do not have permission to update the Python test file. I don't know what step number to assign to a new test. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Thermostat cluster server preset handling so that invoking SetActivePreset applies the preset’s occupied setpoints before updating ActivePresetHandle, and improves error reporting for invalid handles and failed setpoint writes.
Changes:
- Refactors preset lookup to a single-pass
GetMatchingPresetInPresets(...)API that returnsCHIP_ERROR+found. - Updates
SetActivePresetto apply preset heating/cooling setpoints (with limit enforcement) before settingActivePresetHandle, and returnsInvalidCommandwhen the handle is not found. - Propagates setpoint write failures back to the command status and logs the failure.
Comments suppressed due to low confidence (1)
src/app/clusters/thermostat-server/thermostat-server-presets.cpp:356
SetActivePresetwrites occupied setpoints before callingdelegate->SetActivePresetHandle(...). If either setpoint write partially succeeds (e.g. cooling set succeeds but heating set fails) orSetActivePresetHandlefails, the command returns an error but the endpoint may already have modified setpoints, leaving state inconsistent withActivePresetHandle. Consider making this operation transactional (e.g. read and cache existing occupied setpoints, revert on failure) so a non-success return doesn’t leave partial state applied.
CHIP_ERROR err = delegate->SetActivePresetHandle(presetHandle);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "Failed to set ActivePresetHandle with error %" CHIP_ERROR_FORMAT, err.Format());
src/app/clusters/thermostat-server/thermostat-server-presets.cpp
Outdated
Show resolved
Hide resolved
Sum1cares
approved these changes
Mar 15, 2026
|
PR #43587: Size comparison from 2403f0a to eddb5a0 Full report (33 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Thermostat “SetActivePreset” command handling so that selecting an active preset applies that preset’s occupied heating/cooling setpoints (with limit enforcement) and improves error handling when the preset handle is invalid or attribute writes fail.
Changes:
- Refactors preset lookup to a single-pass helper (
GetMatchingPresetInPresets) that returnsCHIP_ERRORplus afoundflag. - Updates
ThermostatAttrAccess::SetActivePresetto apply preset setpoints (withEnforce*SetpointLimits) before updatingActivePresetHandle, and to returnInvalidCommandwhen the handle doesn’t exist. - Propagates and logs failures when writing
OccupiedCoolingSetpoint/OccupiedHeatingSetpoint.
Comments suppressed due to low confidence (1)
src/app/clusters/thermostat-server/thermostat-server-presets.cpp:356
SetActivePresetnow writes occupied setpoints before callingdelegate->SetActivePresetHandle(...). IfSetActivePresetHandlefails, the method returns an error status even though it has already modified one or both setpoints, leaving the device in a partially-updated state (command failed but state changed). Consider making this operation atomic from the client’s perspective (e.g., capture and restore previous setpoints on failure, or defer setpoint writes until after ActivePresetHandle is successfully updated via a mechanism that can’t fail mid-way).
CHIP_ERROR err = delegate->SetActivePresetHandle(presetHandle);
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "Failed to set ActivePresetHandle with error %" CHIP_ERROR_FORMAT, err.Format());
|
PR #43587: Size comparison from 2403f0a to cc54de0 Full report (21 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, psoc6, qpg, realtek, stm32)
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Thermostat preset activation so that selecting an active preset applies the preset’s occupied heating/cooling setpoints (when present) and improves error handling for invalid preset handles and write failures.
Changes:
- Refactors preset lookup into a single-pass
GetMatchingPresetInPresets(...)that returnsCHIP_ERRORplus afoundout-param. - Updates
SetActivePresetto apply preset occupied setpoints (with limit enforcement) before writingActivePresetHandle, and to propagate underlying failures. - Updates pending-preset validation to use the new lookup helper.
Comment on lines
+328
to
+350
| Optional<int16_t> coolingSetpointValue = matchingPreset.GetCoolingSetpoint(); | ||
| if (coolingSetpointValue.HasValue()) | ||
| { | ||
| int16_t constrainedCoolingSetpoint = EnforceCoolingSetpointLimits(coolingSetpointValue.Value(), endpoint); | ||
| Status status = OccupiedCoolingSetpoint::Set(endpoint, constrainedCoolingSetpoint); | ||
| if (status != Status::Success) | ||
| { | ||
| ChipLogError(Zcl, "Failed to set OccupiedCoolingSetpoint with status %u", to_underlying(status)); | ||
| return status; | ||
| } | ||
| } | ||
|
|
||
| Optional<int16_t> heatingSetpointValue = matchingPreset.GetHeatingSetpoint(); | ||
| if (heatingSetpointValue.HasValue()) | ||
| { | ||
| int16_t constrainedHeatingSetpoint = EnforceHeatingSetpointLimits(heatingSetpointValue.Value(), endpoint); | ||
| Status status = OccupiedHeatingSetpoint::Set(endpoint, constrainedHeatingSetpoint); | ||
| if (status != Status::Success) | ||
| { | ||
| ChipLogError(Zcl, "Failed to set OccupiedHeatingSetpoint with status %u", to_underlying(status)); | ||
| return status; | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
This re-entrancy path is now guarded in SetActivePreset: while applying a non-null preset, we increment a scoped presetApplicationDepth, and any callback-driven SetActivePreset(..., nullopt) during that window returns early.
So setpoint writes can still trigger the attribute-changed callback, but they no longer clear ActivePresetHandle mid-application (including deadband-adjustment cascades), avoiding transient clears and extra churn/version bumps.
|
PR #43587: Size comparison from 2403f0a to 44dab5c Full report (34 builds for bl602, bl616, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes Thermostat
SetActivePresetbehavior so that changing the active preset correctly applies the preset’s occupied setpoints before updatingActivePresetHandle.Technical details
SetActivePresetto:OccupiedCoolingSetpoint/OccupiedHeatingSetpointfrom that preset (with existing limit enforcement),ActivePresetHandle.InvalidCommandwhen the requested preset handle is not found.ActivePresetHandle.Impact
Related issues
Testing
Tested with thermostat/linux example:
https://github.com/project-chip/connectedhomeip/tree/master/examples/thermostat/linux
I am not a member of the CSA, so I do not have permission to update the Python test file. I don't know what step number to assign to a new test.
Readability checklist
The checklist below will help the reviewer finish PR review in time and keep the
code readable:
descriptive
“When in Rome…”
rule (coding style)
See: Pull Request Guidelines